CLDR-17014 No code fallbacks for language paths #4254

btangmu · 2024-12-21T04:19:38Z

-Use the extra-paths mechanism instead of code-fallback for language paths

CLDR-17014

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-Use the extra-paths mechanism instead of code-fallback for language paths

macchiati

Because these are all constant paths, they can be statically initiated once, then just added whenever needed.

I'd suggest a separate class, ExtraPaths, with one method .append to(Collection)

btangmu · 2024-12-21T18:54:58Z

a separate class, ExtraPaths

Yes, that sounds good, in fact I was thinking to follow this up with a refactoring PR, to move the extra-path code (not just for languages, but for all extra paths) out of CLDRFile into its own top-level class. It's still not clear to me what part, if any, of extra-path code is locale-specific; the current methods aren't static. It does seem wasteful to re-run code for each locale (each CLDRFile) if it's not locale-specific. Stepping back, though, does the general solution seem right, to use extra paths instead of code fallback for language paths?

There are some test failures, like this, that I still need to study:

  TestCoverageLevel {
    ...
    TestCoverageCompleteness
Error:  (TestCoverageLevel.java:728)  Error: Comprehensive & no exception for path =>	//ldml/localeDisplayNames/languages/language[@type="fa_AF"]

macchiati · 2024-12-22T08:21:05Z

Everything in code-fallback is locale independent and immutable. So that's why you can move it all to the new class, and fetch it with no local parameter. We might be able to just call that when building the root cldrfile.

The other stuff in get extra paths is locale dependent. That could also be moved to the new class, but needs a locale parameter when fetched.

macchiati · 2024-12-22T08:23:03Z

Stepping back, though, does the general solution seem right, to use extra paths instead of code fallback for language paths?

Yes, with the changes I mention.

-Change es-419 to es (419) in localeDisplayName.txt so TestLocaleDisplay passes -Add language path in testExtraPaths so it passes -Remove XMLSource.CODE_FALLBACK_ID, GERMAN, from testGetPaths so it passes

btangmu · 2024-12-22T17:31:23Z

The second commit fixes some but not all test failures.

The remaining test failures are TestCoverageCompleteness and testLSR, both in TestCoverageLevel. These need more investigation.

After the tests are all passing, I'll address refactoring/optimizing with a new class ExtraPaths.

-Create the set of language extra paths only once; related refactoring -Fix another part of testGetPaths so it passes, similar to previous commit -Comments

btangmu · 2024-12-24T03:25:34Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/CLDRFile.java

+    private void getLanguageExtraPaths(Set<String> toAddTo) {
+        Set<String> codes =
+                StandardCodes.make().getSurveyToolDisplayCodes(NameType.LANGUAGE.getNameName());
+        codes.remove(XMLSource.ROOT_ID);


this is wrong, my mistake -- it actually modifies the set in StandardCodes! Need to make a copy of the set. We might want to prevent this kind of bug by making getSurveyToolDisplayCodes return an unmodifiable set, maybe using Set.copyOf.

We currently have this, which doesn't work: CLDRLanguageCodes = CldrUtility.protectCollection(CLDRLanguageCodes);

Fixed by my last commit -- we still should look into Set.copyOf as an alternative to protectCollection

Important: In general, use ImmutableSet.copyOf instead of Set.copyOf. The latter changes the order in the set, which may be important (TreeSet or LinkedHashSets get messed up, and it doesn't hurt for HashSet).

Also, we need to fix CldrUtility.protectCollection(CLDRLanguageCodes); — if it does't work then a lot of other things could go wrong.

-Do not modify SupplementalDataInfo.CLDRLanguageCodes, returned by sc.getGoodAvailableCodes!

btangmu · 2024-12-24T03:50:13Z

tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCoverageLevel.java

@@ -1073,7 +1075,7 @@ public void testLSR() {

        // Get root LSR codes

-        for (String path : root) {
+        for (String path : root.fullIterable()) {


without fullIterable, the loop skipped the extra paths

this may be a concern in general: how often do we loop through a CLDRFile the simple way, which skips extra paths, and what are the consequences and rationale for that?

Good point.

I think in most cases we want to iterate through the extra paths. So we might want make the iteration choice explicit, so that we can check to make sure. Maybe refactor (in separate ticket/PR) as follows.

for (String path : cldrFile) { // make this fail for (String path : cldrFile.withoutExtras()) { // make this do what the line above did for (String path : cldrFile.withExtras()) { // new name for fullIterable

Then we can search for the withoutExtras() calls and make sure that they are right.

The TestVettingDataDriven failure may be related to this. Part of that test involves writing the CLDRFile to a temporary XML file and then reading it back to verify the newly voted-on value, and that's where it's failing. It looks like the language paths, which are now extra paths, don't get written to the file. To test that, I'm starting a new branch from main that will just have a few lines added to TestSTFactory.xml...

See #4256

If the same failure happens there that I see locally, it seems to indicate a long-existing bug where CldrXmlWriter doesn't write extra paths

btangmu · 2024-12-24T03:51:50Z

The test failures all seem fixed now except for TestVettingDataDriven

macchiati · 2024-12-25T13:12:42Z

Well, it mustn't write the extra paths, because the have null values, which cannot be serialized. With the changes to code-fallback to not have values, there is a difference in behavior. No surprise that that will require changes.

…

On Tue, Dec 24, 2024, 19:13 Tom Bishop ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCoverageLevel.java <#4254 (comment)>: > @@ -1073,7 +1075,7 @@ public void testLSR() { // Get root LSR codes - for (String path : root) { + for (String path : root.fullIterable()) { See #4256 <#4256> If the same failure happens there that I see locally, it seems to indicate a long-existing bug where CldrXmlWriter doesn't write extra paths — Reply to this email directly, view it on GitHub <#4254 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMEZ56VKN27SAOLQSYT2HGP27AVCNFSM6AAAAABUAHVCSGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRSGE2TSMBXGU> . You are receiving this because you commented.Message ID: ***@***.***>

btangmu · 2024-12-25T18:41:01Z

it mustn't write the extra paths, because the have null values, which cannot be serialized

I wouldn't expect extra paths with null values to be written to disk, but I did expect extra paths with non-null values to be written to disk. The test failures here and in #4256 show that an extra path, originally having a null value (or no value), can get a value through voting, and the CLDRFile is updated successfully in memory, but it fails to get written to disk. The failure is happening in #4256 even though there is no change in that PR except for the test itself -- it's for a metazone path and doesn't involve any change to language paths, code fallback, etc.

The path //ldml/dates/timeZoneNames/metazone[@type="Alaska"]/long/generic does have a value in common/main/fr.xml for example. Maybe it's no longer called an "extra path" at that point; maybe part of my confusion is about terminology. Anyway I'm wondering how it got into fr.xml if not by voting followed by CldrXmlWriter.write.

macchiati · 2024-12-26T14:14:15Z

Hmmm. Yes, the extra paths are no longer 'extra' once they have values. Not sure why they are failing

to get written to disk.

There are plenty of other cases in the existing code that worked fine in the past. Guess the only recourse is to walk through what's happening in a debugger.

…

On Wed, Dec 25, 2024, 19:41 Tom Bishop ***@***.***> wrote: it mustn't write the extra paths, because the have null values, which cannot be serialized I wouldn't expect extra paths with null values to be written to disk, but I did expect extra paths with non-null values to be written to disk. The test failures here and in #4256 <#4256> show that an empty path, originally having a null value (or no value), can get a value through voting, and the CLDRFile is updated successfully in memory, but it fails to get written to disk. The failure is happening in #4256 <#4256> even though there is no change in that PR except for the test itself -- it's for a metazone path and doesn't involve any change to language paths, code fallback, etc. The path ***@***.***="Alaska"]/long/generic does have a value in common/main/fr.xml for example. Maybe it's no longer called an "extra path" at that point; maybe part of my confusion is about terminology. Anyway I'm wondering how it got into fr.xml if not by voting followed by CldrXmlWriter.write. — Reply to this email directly, view it on GitHub <#4254 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMFPDGWMH6NBW4WM55D2HL35HAVCNFSM6AAAAABUAHVCSGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRRHE3TGNZZGE> . You are receiving this because you commented.Message ID: ***@***.***>

CLDR-17014 No code fallbacks for language paths

f846a3c

-Use the extra-paths mechanism instead of code-fallback for language paths

btangmu self-assigned this Dec 21, 2024

macchiati requested changes Dec 21, 2024

View reviewed changes

CLDR-17014 Revise some tests so they pass

7efc8d8

-Change es-419 to es (419) in localeDisplayName.txt so TestLocaleDisplay passes -Add language path in testExtraPaths so it passes -Remove XMLSource.CODE_FALLBACK_ID, GERMAN, from testGetPaths so it passes

btangmu added 3 commits December 22, 2024 23:00

CLDR-17014 Move code for language extra paths to new ExtraPaths.java

cdb5251

-Create the set of language extra paths only once; related refactoring -Fix another part of testGetPaths so it passes, similar to previous commit -Comments

CLDR-17014 Debugging and minor optimization

aaf5e63

CLDR-17014 Fix testLSR using root.fullIterable()

e3f2f32

btangmu commented Dec 24, 2024

View reviewed changes

CLDR-17014 Fix populateLanguagePaths bug

518b994

-Do not modify SupplementalDataInfo.CLDRLanguageCodes, returned by sc.getGoodAvailableCodes!

btangmu commented Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDR-17014 No code fallbacks for language paths #4254

CLDR-17014 No code fallbacks for language paths #4254

btangmu commented Dec 21, 2024

macchiati left a comment

btangmu commented Dec 21, 2024

macchiati commented Dec 22, 2024

macchiati commented Dec 22, 2024

btangmu commented Dec 22, 2024

btangmu Dec 24, 2024

btangmu Dec 24, 2024

macchiati Dec 24, 2024

macchiati Dec 24, 2024

btangmu Dec 24, 2024

macchiati Dec 24, 2024

btangmu Dec 24, 2024

btangmu Dec 24, 2024

btangmu commented Dec 24, 2024

macchiati commented Dec 25, 2024 via email

btangmu commented Dec 25, 2024 •

edited

Loading

macchiati commented Dec 26, 2024 via email

CLDR-17014 No code fallbacks for language paths #4254

Are you sure you want to change the base?

CLDR-17014 No code fallbacks for language paths #4254

Conversation

btangmu commented Dec 21, 2024

macchiati left a comment

Choose a reason for hiding this comment

btangmu commented Dec 21, 2024

macchiati commented Dec 22, 2024

macchiati commented Dec 22, 2024

btangmu commented Dec 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btangmu commented Dec 24, 2024

macchiati commented Dec 25, 2024 via email

btangmu commented Dec 25, 2024 • edited Loading

macchiati commented Dec 26, 2024 via email

btangmu commented Dec 25, 2024 •

edited

Loading